Skip to content

fix(mcp): roll the dependency contracts, so the tool classes can be built - #21

Merged
Snider merged 1 commit into
mainfrom
fix/roll-mcp-dependency-contracts
Aug 8, 2026
Merged

fix(mcp): roll the dependency contracts, so the tool classes can be built#21
Snider merged 1 commit into
mainfrom
fix/roll-mcp-dependency-contracts

Conversation

@Snider

@Snider Snider commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

All 40 MCP tool classes have been unconstructible in every environment. Not unregistered — unbuildable.

php -r 'new \Core\Mod\Agentic\Mcp\Tools\Agent\Brain\BrainRemember();'
FATAL: Trait "Core\Mcp\Tools\Concerns\ValidatesDependencies" not found

AgentTool does use ValidatesDependencies and implements HasDependencies, and neither symbol has ever existed in this repository. They live in dappcore/mcp, which this package deliberately does not depend on.

The dead $listens event registration — which meant nothing ever tried to instantiate a tool — was anaesthetic, not a second bug. Make registration live without this fix and the suite goes from 156 failures to 1321.

The fix

The design is a local copy of Core\Mcp, not a dependency on it, so the fix is to finish the copy. Three contracts roll in unchanged from dappcore/mcp (now canonical after mcp#20): DependencyType, ToolDependency, HasDependencies, each gaining the SPDX header this repo requires.

Two could not be rolled verbatim

Upstream's ToolDependencyService and this repo's share a name and not an API.

MissingDependencyException — upstream takes (string $toolName, array $missingDependencies, array $suggestedOrder) and composes its own message. This service raises it as new $exceptionClass($message), one argument. Copying the upstream signature would have replaced a class-not-found with an ArgumentCountError the first time a dependency went unmet. Written message-first, detail optional.

ValidatesDependencies — upstream calls methods that don't exist here, and passes named arguments to a variadic:

upstream trait calls this service provides
checkDependencies(...) canExecute($toolId, $context, $args, $session)
getMissingDependencies(...) missing($toolId, $context, $args, $session)
validateDependencies(sessionId:, toolName:, args:) validateDependencies(mixed ...$arguments) — named args bind to nothing

Written against the API this repo actually has, positionally, returning the service's own {tool, type, key, message} rows rather than pretending to hand back ToolDependency objects.

Two supporting fixes fell out

AgentToolRegistry imported Core\Mcp\Services\ToolDependencyService, which resolves to this repo's php/Mcp/Services/ToolDependencyService.php — a file declaring Core\Mod\Agentic\Mcp\Services\ToolDependencyService — so autoloading it raised "Cannot redeclare". It now imports the class that is actually there.

normaliseDependency() met ToolDependency objects with get_object_vars(), handing the array branch a DependencyType enum where it casts to string, and losing the text because the object calls it description while the service reads message. It now goes through toArray() and maps the field across.

Sixteen tools declare real dependencies via ToolDependency::contextExists(), so this path is live, not hypothetical.

Receipts

result
tool classes constructing 40 of 40 — every one instantiated
suite 131 failed, 1190 passed — from 156 / 1165
fixed exactly the 25-test ValidatesDependencies cluster
new failures zero, confirmed by diffing failing test names either side

Gate re-verified after linting, not before.

🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io

…uilt

AgentTool does `use ValidatesDependencies` and `implements HasDependencies`,
and neither symbol has ever existed in this repository — they live in
dappcore/mcp, which this package deliberately does not depend on. Every class
extending AgentTool was therefore fatal on load:

    php -r 'new \Core\Mod\Agentic\Mcp\Tools\Agent\Brain\BrainRemember();'
    FATAL: Trait "Core\Mcp\Tools\Concerns\ValidatesDependencies" not found

All forty registered tool classes, unconstructible in every environment. Not
"unregistered" — unbuildable. The dead $listens event registration that meant
nothing ever tried to instantiate one was anaesthetic, not a second bug: make
registration live without this and the suite goes from 156 failures to 1321.

The design is a local copy of Core\Mcp, not a dependency on it, so the fix is
to finish the copy. Three contracts roll in unchanged from dappcore/mcp, which
now holds the canonical versions: DependencyType, ToolDependency and
HasDependencies. Each gains the SPDX header this repo requires.

Two could NOT be rolled verbatim, because upstream's and this repo's
ToolDependencyService share a name and not an API.

MissingDependencyException upstream takes
(string $toolName, array $missingDependencies, array $suggestedOrder) and
composes its own message. This service raises it as `new $exceptionClass($message)`
— one argument. Copying the upstream signature would have replaced a
class-not-found with an ArgumentCountError the first time a dependency went
unmet. It is written message-first, with the detail as optional arguments.

ValidatesDependencies upstream calls checkDependencies() and
getMissingDependencies(), which do not exist here — the equivalents are
canExecute() and missing() — and passes named arguments to a method declared
`validateDependencies(mixed ...$arguments)`, where they bind to nothing. It is
written against the API this repo actually has, positionally, and returns the
service's own {tool, type, key, message} rows rather than pretending to hand
back ToolDependency objects.

Two supporting fixes fall out. AgentToolRegistry imported
Core\Mcp\Services\ToolDependencyService, which resolves to this repo's
php/Mcp/Services/ToolDependencyService.php — a file declaring
Core\Mod\Agentic\Mcp\Services\ToolDependencyService — so autoloading it raised
"Cannot redeclare". It now imports the class that is actually there. And
normaliseDependency() met ToolDependency objects with get_object_vars(), which
hands the array branch a DependencyType enum where it casts to string, and
loses the text because the object calls it description where the service reads
message; it now goes through toArray() and maps the field across.

Sixteen tools declare real dependencies via ToolDependency::contextExists(),
so this path is live, not hypothetical.

Receipts: all 40 tool classes construct, verified by instantiating every one.
Suite 131 failed / 1190 passed, from 156 / 1165 — exactly the 25-test
ValidatesDependencies cluster fixed, zero new failures, confirmed by diffing
failing test names either side.

Co-Authored-By: Virgil <virgil@lethean.io>
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Snider, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cc1d85ae-9466-4fec-a7ca-19543528ae5a

📥 Commits

Reviewing files that changed from the base of the PR and between a0debd8 and de5600b.

📒 Files selected for processing (7)
  • php/Mcp/Dependencies/DependencyType.php
  • php/Mcp/Dependencies/HasDependencies.php
  • php/Mcp/Dependencies/ToolDependency.php
  • php/Mcp/Exceptions/MissingDependencyException.php
  • php/Mcp/Services/ToolDependencyService.php
  • php/Mcp/Tools/Concerns/ValidatesDependencies.php
  • php/Services/AgentToolRegistry.php

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

@Snider
Snider merged commit 9c9467d into main Aug 8, 2026
5 of 8 checks passed
@Snider
Snider deleted the fix/roll-mcp-dependency-contracts branch August 8, 2026 11:31
Snider added a commit that referenced this pull request Aug 8, 2026
* wip: consolidate tool registries (blocked on Core\Mcp contracts)

Absorbs listTools/resolve/buildDependencyGraph/call into AgentToolRegistry,
rewires McpAgentServerCommand and ToolDependencyService onto it, and deletes
Mcp\Services\ToolRegistry.

Deliberately NOT pushed. Moving the fill off $listens — the other half of the
fix — makes register() construct the tool classes, which fatals on the missing
Core\Mcp\Tools\Concerns\ValidatesDependencies trait and takes the suite from
156 failed to 1321. The registries can only usefully merge once agent consumes
dappcore/mcp and the tools become constructible.

19 tests still fail here: their fixture is a duck-typed anonymous class and the
surviving registry requires a real AgentToolInterface. Migrating them belongs
with the change that turns the server on.

* fix(mcp): one tool registry, filled at boot, read by the server

The agent MCP server advertised no tools. Three faults, each of which alone
was enough, and a green suite that noticed none of them.

Boot filled Core\Mod\Agentic\Services\AgentToolRegistry. McpAgentServerCommand
read Core\Mod\Agentic\Mcp\Services\ToolRegistry — a different class, never
bound, so Laravel handed the command a fresh empty instance on every
resolution. tools/list returned []; tools/call found nothing. Two registries
meant two answers to "what tools exist", and the server asked the one nobody
filled.

Boot filled its registry from the McpToolsRegistering event via $listens, which
ModuleScanner populates by scanning app/Core|Mod|Website. Under vendor/ that is
dead, so the event never fired and the registry it did fill was empty anyway.

And every tool class was fatal on load until #21, so even a correct
registration would have thrown on the first `new`. That one masked the other
two: nothing ever tried to construct a tool, so nothing ever failed loudly.

ToolRegistry is deleted and its capability absorbed: listTools(), resolve() and
buildDependencyGraph() return ToolMetadata built from the registered tools,
call() invokes one without the permission and dependency checks execute()
applies — kept separate because the stdio transport has no API key to check
scopes against and runs its own quota and audit passes around it. The
duplicate-name guard comes across too: two tools claiming one name is a wiring
mistake, and silently keeping the last one means the surface serves whichever
file loaded second.

The fill moves from the event into register(), the same lifecycle-independent
path used for resources, and is idempotent so a host that still delivers the
event cannot double-register.

Nineteen tests registered duck-typed anonymous classes into the loose registry.
They now implement AgentToolInterface — which they arguably always should have,
since it is the contract the tools they stand in for satisfy. One test goes
rather than being migrated: it asserted that a payload without a callable
handler is rejected, and register() is now typed, so no array can reach that
validation. There is no code path left that produces the behaviour it asserted.

Guarded against recurrence by the test that was missing all along: on a plain
booted application, registering nothing of its own, a tool constructs, the
registry is non-empty, listTools() contains plan_create, session_start and
brain_remember, and the binding is one shared instance. McpAgentServerCommandTest
passed throughout the outage because its beforeEach supplied a tool — it tested
the plumbing with a registry the test had filled, which is precisely the blind
spot.

Receipts: registry holds 40 tools after a real boot, listTools() returns the
same 40, plan_create among them. Suite 131 failed / 1193 passed, from 131 /
1190 — four guards added, one obsolete test removed, zero regressions confirmed
by diffing failing test names.

Co-Authored-By: Virgil <virgil@lethean.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant